Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impose pre-determined routes instead of announcing them in the manifest #91

Merged
merged 3 commits into from
Nov 10, 2022

Conversation

wetneb
Copy link
Member

@wetneb wetneb commented Jul 26, 2022

This is a first step towards #84: instead of leaving the suggest, preview and property proposal paths up to the implementer, those paths are pre-determined by the spec.

This was also the occasion to remove the flyout: I had to do this when changing the manifest structure. Sorry to be bundling that up in the same PR, I should have done another one about this earlier. Therefore it closes #42.

It also closes #32 in the same go.

@wetneb wetneb requested a review from fsteeg July 26, 2022 16:28
@@ -638,10 +651,11 @@ <h3>Preview Metadata</h3>
</section>
<section>
<h3>Preview Queries</h3>
<p>A preview service is queried by resolving the <a>preview URL</a> for an <a>entity</a>. The URL must resolve to an HTML document.
<p>A preview service is queried by resolving the URI template <code>/preview?id={id}</code> relative to the reconciliation endpoint, where <code>id</code> is subsituted by the <a>entity</a> identifier. The URL must resolve to an HTML document,
which MUST be viewable in an <code>iframe</code> HTML element whose dimensions are determined by the <a>preview metadata</a>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be CAN instead? I mean some clients don't necessarily need an iframe, it's up to them how to interact with data coming from a standardized api spec?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That MUST does not force clients to embed the preview in an iframe, it only ensures that the server makes that possible. I would be fine with reformulating this to say something like "an HTML page which MUST be viewable in a viewport whose dimensions are…", if you want to avoid the term iframe…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes would be better to rephrase like that I think.

Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see so much text in the spec removed (+96 −246)!

Made one small inline suggestion for a missing removal.

Approving this to keep focused on the pre-determined routes vs. announcing them, but since this removes the optional GET routes and changes the way the main reconcile and extend routes work, this might be the right point to reconsider if we really want to use POST here. In particular if our goal is to get closer to REST principles, I'd think that reconcile and extend should be GET routes, since they only query data and don't change any data on the server.

(BTW there seems to be an upcoming new verb for 'GET with a body', currently called SEARCH.)

@wetneb
Copy link
Member Author

wetneb commented Aug 9, 2022

Thanks, good catch!

For GET vs POST, I guess we are still limited by the fact that:

  • passing GET parameters in the URL is subject to length limits
  • passing GET parameters in the body is a pretty uncommon / weird thing to do, which might require contortions depending on the framework used for the server or client

But I get that it might also be weird to use POST in some web frameworks, typically those which enforce some sort of CSRF protection on such endpoints (which would need to be disabled for our purposes).

@thadguidry
Copy link
Contributor

I'd prefer that we stay with HTTP POST as we have defined in the latest draft.

This would allow a bit more flexibility in the kinds of entities that could potentially be reconciled. For instance, I wonder if it should be optional for a service to use multipart/form-data for binary data and not only application/x-www-form-urlencoded but haven't thought about it much beyond use cases of machine learning with file data such as images or sound clips with associated metadata.

@acka47
Copy link
Member

acka47 commented Aug 11, 2022

BTW there seems to be an upcoming new verb for 'GET with a body', currently called SEARCH.

By now it's called QUERY method. I think this is the most up-to-date draft: https://www.rfc-editor.org/rfc/internet-drafts/draft-ietf-httpbis-safe-method-w-body-03.html (See also this post for some background: https://evertpot.com/get-request-bodies/)

@wetneb
Copy link
Member Author

wetneb commented Aug 11, 2022

This would allow a bit more flexibility in the kinds of entities that could potentially be reconciled. For instance, I wonder if it should be optional for a service to use multipart/form-data for binary data and not only application/x-www-form-urlencoded but haven't thought about it much beyond use cases of machine learning with file data such as images or sound clips with associated metadata.

Actually we want to migrate out of such content-types because they are not as REST-friendly as just application/json.

@wetneb wetneb mentioned this pull request Nov 10, 2022
@wetneb wetneb merged commit e6e6132 into master Nov 10, 2022
fsteeg added a commit that referenced this pull request Dec 6, 2022
URL is now built by resolving URI template: `/preview?id={id}`
fsteeg added a commit that referenced this pull request Jul 13, 2023
…st (#91)

* Impose pre-determined routes rather than announcing them in the manifest, for #84

* Remove mention of iframe

* Remove the mention of form elements.

Co-authored-by: Fabian Steeg <[email protected]>

Co-authored-by: Fabian Steeg <[email protected]>
fsteeg added a commit that referenced this pull request Jul 13, 2023
URL is now built by resolving URI template: `/preview?id={id}`
@wetneb wetneb deleted the 84-restification branch April 6, 2024 14:57
@fsteeg fsteeg mentioned this pull request Sep 11, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove flyout services Submitting queries as JSON without URL-encoding them
4 participants